-
Notifications
You must be signed in to change notification settings - Fork 77
fix: aria-live and aria-describedby on fieldset element. #431
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: aria-live and aria-describedby on fieldset element. #431
Conversation
8fdc5da
to
878247c
Compare
@@ -197,18 +197,11 @@ export const Fieldset = memo( | |||
<div | |||
className={fr.cx("fr-messages-group")} | |||
id={messagesWrapperId} | |||
aria-live="assertive" | |||
aria-live={state === "error" ? "assertive" : undefined} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Le aria-live assertive est à renseigner seulement si c'est une erreur.
Désolé pour le délais, je peux pas merge en l'état car ça introduit un breaking change : les id des états |
878247c
to
4758c34
Compare
Merci pour ton retour @ddecrulle. J'ai modifié ma PR pour ne pas avoir d'impact sur les id des états error et success. Cela n'impacte pas le correctif sur le aria-live assertive. |
4758c34
to
daafac4
Compare
case "success": | ||
return successDescId; | ||
case "info": | ||
return infoDescId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'ai ajouté un id sur l'info car quand on a une info, il faut la lier avec le aria-describedby
No description provided.